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.
Comment | File | Size | Author |
---|---|---|---|
#255 | vt-screenshot.png | 42.23 KB | Les Lim |
#253 | jumping_tabs_half.gif | 302.79 KB | matt2000 |
#225 | group.mdown_.txt | 2.55 KB | kkaefer |
#224 | #group.mdown_.txt | 2.55 KB | kkaefer |
#214 | vertical_tabs_changelog.patch | 1007 bytes | keith.smith |
Comments
Comment #1
dmitrig01 CreditAttribution: dmitrig01 commentedNote: this is a partial screenshot that cuts some stuff off. you'll have to install it yourself to play with it
Comment #2
Dave ReidSeems 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.
Comment #3
yched CreditAttribution: yched commented"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.
Comment #4
Dries CreditAttribution: Dries commentedI 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.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedThis 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:
Comment #6
birdmanx35 CreditAttribution: birdmanx35 commentedSubscribing.
Comment #7
dmitrig01 CreditAttribution: dmitrig01 commented@#5
I can't deal with #1, but as for #2: the markup is structured like fieldsets.
Comment #8
dmitrig01 CreditAttribution: dmitrig01 commentedWe can get theme support in later. Here's the patch with documentation - please review.
Comment #9
keith.smith CreditAttribution: keith.smith commentedI know this is CNW, but in case you reroll:
"scallback"? :)
We don't normally abbreviate "JavaScript" as "JS". Also, "it's" should be "its" here.
"description"
End in period.
"restul"
Comment #10
dmitrig01 CreditAttribution: dmitrig01 commentedfixed. please review
Comment #11
keith.smith CreditAttribution: keith.smith commentedYep. Except for the it's/its thing.
Comment #12
moshe weitzman CreditAttribution: moshe weitzman commentedI like the UI and something like this is core worthy IMO. I have a couple significant issues though.
Comment #13
alpritt CreditAttribution: alpritt commentedComment #14
dmitrig01 CreditAttribution: dmitrig01 commentedComment #15
nielsvm CreditAttribution: nielsvm commentedsubscribing
Comment #16
mfer CreditAttribution: mfer commentedOn 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?
Comment #17
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #18
alpritt CreditAttribution: alpritt commented@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:
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.
Comment #19
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #20
alpritt CreditAttribution: alpritt commented@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.
Comment #21
mfer CreditAttribution: mfer commentedThe 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?Comment #22
alpritt CreditAttribution: alpritt commented@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.
Comment #23
mfer CreditAttribution: mfer commented@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:
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.
Comment #24
mfer CreditAttribution: mfer commentedHere's a patch with all the files to help the css along.
Comment #25
alpritt CreditAttribution: alpritt commentedThis is just a quick thing and has some problems, but I'm thinking keep the design very simple; especially for the base theming.
Comment #26
dmitrig01 CreditAttribution: dmitrig01 commentedThe summaries were made into links so the whole tab was clickable
Comment #27
Owen Barton CreditAttribution: Owen Barton commentedSubscribing
Comment #28
Dries CreditAttribution: Dries commentedComment #29
ximo CreditAttribution: ximo commented@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?
Comment #30
SeanBannister CreditAttribution: SeanBannister commented+1
Comment #31
webchickThis is a favourite-of-webchick, too. :)
Comment #32
webchickComment #33
mfer CreditAttribution: mfer commentedThe holdup on this, as far as I am aware, is addressing the accessibility concerns. Does anyone know the 'right' way to address these?
Comment #34
Owen Barton CreditAttribution: Owen Barton commentedI 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.
Comment #35
Gurpartap Singh CreditAttribution: Gurpartap Singh commentedComment #36
XanoSubscribing.
Great idea, Dmitri!
Comment #37
lut4rp CreditAttribution: lut4rp commentedComment #38
int CreditAttribution: int commentedSubscribing
Comment #39
catchComment #41
dmitrig01 CreditAttribution: dmitrig01 commentedrerolled
Comment #43
quicksketchNot sure how I missed this. Sub.
Comment #44
alexanderpas CreditAttribution: alexanderpas commentedComment #45
karschsp CreditAttribution: karschsp commentedsubscribe
Comment #46
catchPatch didn't add the new js or css files. edit: but it's still not working for me. Leaving at CNR for another look.
Comment #48
catchWith menu.js this time.
Comment #49
catchThe 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.
Comment #50
dmitrig01 CreditAttribution: dmitrig01 commentedFixed it
Comment #51
Les LimJust 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 .
Comment #52
Les LimHere'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.
Comment #53
Les LimOnce 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.
Comment #54
catchSame patch with weight 0 as described in #53. Please test.
Comment #55
catchNot a happy upload form:
http://ca.tchpole.net/sites/ca.tchpole.net/files/vertical_tabs-323112-53...
Comment #56
ejort CreditAttribution: ejort commentedsubscribe
Comment #57
catchTrying the upload a different way.
Comment #58
dmitrig01 CreditAttribution: dmitrig01 commentedneeds a
(jQuery)
at the endComment #59
Les LimSorry, 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.
Comment #60
yoroy CreditAttribution: yoroy commentedtagging. Getting the idea we're nearly there with this.
Comment #61
bennybobw CreditAttribution: bennybobw commentedSo 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:
vs.
That's why we're getting some wonkiness on save/preview.
Comment #62
bennybobw CreditAttribution: bennybobw commentedHere's a patch to try to fix the JSON issue.
Comment #63
bennybobw CreditAttribution: bennybobw commentedline 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.
Comment #64
bennybobw CreditAttribution: bennybobw commented.apply requires an array, not a string as the second argument. Patch attached. This fixes the error in IE.
Comment #65
dmitrig01 CreditAttribution: dmitrig01 commentedyour patch didn't add the new files. see http://drupal.org/patch/create#new
Comment #66
bennybobw CreditAttribution: bennybobw commentedSo 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.
Comment #67
bennybobw CreditAttribution: bennybobw commentedCrap. Sorry bad patch. I was wondering why mine was so much smaller.
Comment #68
bennybobw CreditAttribution: bennybobw commentedReroll with new files....
I think I got them all....
Comment #69
dmitrig01 CreditAttribution: dmitrig01 commentedSo how does this solve the problem?
Comment #70
bennybobw CreditAttribution: bennybobw commentedThere 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.
Comment #71
PixelClever CreditAttribution: PixelClever commentedsubscribing
Comment #72
ultimateboy CreditAttribution: ultimateboy commentedSmall 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.
Comment #74
Bevan CreditAttribution: Bevan commentedOh cool! I can't believe I only just found out my Season of Usability work haas finally fruitioned into a core patch!
Comment #75
dmitrig01 CreditAttribution: dmitrig01 commentedLet's run this by the bot again.
Comment #76
dmitrig01 CreditAttribution: dmitrig01 commentedAlso - it seems the fail is COMPLETELY unrelated - "user language settings". I think the fail is there before and after
Comment #77
kkaefer CreditAttribution: kkaefer commentedHere are some issues I found:
Comment #78
kkaefer CreditAttribution: kkaefer commentedPatch 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.
Comment #79
kkaefer CreditAttribution: kkaefer commentedfunctionality works in IE7 but styling is a bit off. This might or might not be fixed in a future version of the patch.
Comment #80
quicksketchI'm working on fixing the problems described in #77.
Comment #81
quicksketchThis 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.
Comment #82
yoroy CreditAttribution: yoroy commentedbot, if you'd be so kind…
Comment #83
kkaefer CreditAttribution: kkaefer commentedThe 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.
Comment #84
quicksketchHmm, 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.
Comment #85
kkaefer CreditAttribution: kkaefer commentedI 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.
Comment #86
kkaefer CreditAttribution: kkaefer commentedWhen 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%?
Comment #87
Les LimWe could alternatively apply CSS conditionally through jQuery browser sniffing.
Comment #88
quicksketchIt 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.
Comment #89
ejort CreditAttribution: ejort commentedJust 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".
Comment #90
kkaefer CreditAttribution: kkaefer commentedThis CSS reproduces the display: table effect in IE6,7, Firefox3 and WebKit. Garland styling is still missing.
Comment #91
quicksketchWow, 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.
Comment #92
kkaefer CreditAttribution: kkaefer commentedFixed 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.
Comment #93
kkaefer CreditAttribution: kkaefer commentedUploading doesn't work but the tests don't seem to catch it. This needs a test for uploading.
Comment #94
kkaefer CreditAttribution: kkaefer commentedComment #95
Les LimAn 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.
Comment #96
dmitrig01 CreditAttribution: dmitrig01 commentedTestingbot doesn't do JS, that's why it works.
Comment #97
dmitrig01 CreditAttribution: dmitrig01 commentedI 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
Comment #98
chx CreditAttribution: chx commentedSuch 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.
Comment #99
kika CreditAttribution: kika commentedSorry 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...
...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.
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:
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):
[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?
Comment #100
Frando CreditAttribution: Frando commentedWell, 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!
Comment #101
kika CreditAttribution: kika commentedAnother 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?
Comment #102
kika CreditAttribution: kika commentedI 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?
Comment #103
Frando CreditAttribution: Frando commented#102: Good point. Moved the drupal_add_css and drupal_add_js calls into the theme function.
Comment #104
kika CreditAttribution: kika commentedagain, 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.
Comment #105
kkaefer CreditAttribution: kkaefer commented@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.
Comment #106
jrabeemer CreditAttribution: jrabeemer commentedBug:
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.
Comment #107
kkaefer CreditAttribution: kkaefer commented@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).
Comment #108
dmitrig01 CreditAttribution: dmitrig01 commented@kkaefer I doubt it does, and i'm not sure this is a browser thing. Maybe autocomplete should do it.
Comment #109
jrabeemer CreditAttribution: jrabeemer commentedI'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() {
Comment #112
kkaefer CreditAttribution: kkaefer commentedUpdated the patch to most recent Drupal HEAD.
Refactored the way description callbacks are attached. Instead of
we now just write
to attach the description callback to a fieldset. For this to work,
vertical-tabs.js
has to be included before the calls toDrupal.verticalTab.addDescription()
. Thevertical-tabs.js
now has a weight ofJS_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.
Comment #113
kkaefer CreditAttribution: kkaefer commentedAs per chx’ request, I removed
#process
ing 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.Comment #114
kkaefer CreditAttribution: kkaefer commentedThis is an alternative patch and requires http://drupal.org/node/409464. It fixes the Pareview problem.m
Comment #115
kkaefer CreditAttribution: kkaefer commentedPatch from #114 with missing files. If RTBC, please use patch #113.
Comment #117
chx CreditAttribution: chx commentedI 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.
Comment #118
catchextra line break?
no period.
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.
Comment #119
kkaefer CreditAttribution: kkaefer commentedThe 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.
Comment #120
quicksketchRegarding "$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:
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.
Comment #121
kkaefer CreditAttribution: kkaefer commentedThis 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: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 respectivedrupal_add_*
function or an array with parameters to pass to that function. This solves the form caching problem outlined in #112.Comment #122
chx CreditAttribution: chx commentedComment #123
kkaefer CreditAttribution: kkaefer commentedSome minor improvements:
#default_tab
.Comment #124
quicksketchThis 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.
Comment #125
kkaefer CreditAttribution: kkaefer commentedThe 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.
Comment #126
kkaefer CreditAttribution: kkaefer commentedThis is a screenshot of the RTL interface:
Comment #127
kbahey CreditAttribution: kbahey commentedchx 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
Comment #128
keith.smith CreditAttribution: keith.smith commentedAre the untranslated bits in #126 just strings that aren't translated in that language pack?
Comment #129
kkaefer CreditAttribution: kkaefer commentedYes, 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).
Comment #130
int CreditAttribution: int commentedI think that, we can now commit this, and after that, we can improve others functionalities if are any more left...
Comment #131
webchickLet's get some folks testing this in different versions of Firefox, IE, Safari, Opera, etc. and post back with your results.
Comment #132
andreiashu CreditAttribution: andreiashu commentedI'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.
Comment #133
andreiashu CreditAttribution: andreiashu commentedTested 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.
Comment #134
XanoJust something that crossed my mind: how well does this work with rather wide content, like at admin/build/modules?
Comment #135
andreiashu CreditAttribution: andreiashu commented@Xano I don't understand. At admin/build/modules I don't get vertical tabs.
Comment #136
kkaefer CreditAttribution: kkaefer commentedPatch 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:
:(Comment #137
webchickIE issues need fixing. Thanks a lot for testing, andreiashu !
Comment #138
kkaefer CreditAttribution: kkaefer commentedPatch #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.
Comment #139
andreiashu CreditAttribution: andreiashu commentedI could also retest on ie7 + FF + chrome on Vista.
Everything seems oki now :)
Great work !
Comment #140
XanoI 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.
Comment #141
andreiashu CreditAttribution: andreiashu commented@Xano: read the first two lines of this issue's description :)
Comment #142
Dries CreditAttribution: Dries commentedI 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.
Comment #143
andreiashu CreditAttribution: andreiashu commented@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 ?
Comment #144
catchNo phpdoc here:
Comment #145
kkaefer CreditAttribution: kkaefer commentedAdded 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.
Comment #146
webchickkkaefer: Could you post a screenshot of the problem?
Comment #147
kkaefer CreditAttribution: kkaefer commentedScreencast: http://dump.kkaefer.com/vtabs.mov
Comment #148
catchLooked 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.
Comment #149
jrabeemer CreditAttribution: jrabeemer commentedIs there any way to trigger a form event to force IE6 to reformat? That would require some JS and no CSS hacks.
Comment #150
chx CreditAttribution: chx commentedI do not think such a minor problem should hold such a major UX advantage.
Comment #151
Les LimInstead 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):
The following snippet added just before the return would do it:
Patch rerolled and attached.
Comment #152
int CreditAttribution: int commentedComment #153
chx CreditAttribution: chx commentedhttp://vt.drupal4hu.com/node/add/page is updated. It's switched to RTL.
Comment #154
Bojhan CreditAttribution: Bojhan commentedFrom 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.
Comment #155
Dries CreditAttribution: Dries commentedJust 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.
Comment #156
Dries CreditAttribution: Dries commentedSlightly 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.
Comment #157
jrabeemer CreditAttribution: jrabeemer commentedThat'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
Comment #158
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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
Comment #159
rsvelko CreditAttribution: rsvelko commented@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.
Comment #160
Anonymous (not verified) CreditAttribution: Anonymous commented@rsvelko - you're right, this is off-topic. Created a new thread here: http://tinyurl.com/c5bo7m
Comment #161
kkaefer CreditAttribution: kkaefer commentedWorking on a better patch...
Comment #162
kkaefer CreditAttribution: kkaefer commentedThis is a major update:
$('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).#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.#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
.Comment #163
kkaefer CreditAttribution: kkaefer commentedBug: when previewing, the tabs appear twice.
Comment #164
kkaefer CreditAttribution: kkaefer commentedFixes the bug where tabs appear twice when previewing and adds more documentation to JavaScript.
Comment #165
chx CreditAttribution: chx commentedhttp://vt.drupal4hu.com/node/add/page upped with #164
Comment #166
ultimateboy CreditAttribution: ultimateboy commentedDuring 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)
Comment #167
wretched sinner - saved by grace CreditAttribution: wretched sinner - saved by grace commented@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)
Comment #168
kkaefer CreditAttribution: kkaefer commentedMaking 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.
Comment #169
tstoecklerI 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.
Comment #170
int CreditAttribution: int commentedComment #171
Les LimHere'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.
Comment #172
kkaefer CreditAttribution: kkaefer commentedThanks for catching that. The new page properly escapes input.
Comment #173
kkaefer CreditAttribution: kkaefer commentedComment #174
catchThe 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.
Comment #175
kkaefer CreditAttribution: kkaefer commentedAdded @element for the theme function. I don't know what spaces you are talking about next to @return, though.
Comment #176
cwgordon7 CreditAttribution: cwgordon7 commentedkkaefer - I believe that catch means that comment blocks like this:
Should be formatted like this:
Comment #177
kkaefer CreditAttribution: kkaefer commentedRemoved the space.
Comment #178
int CreditAttribution: int commentedI 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....
Comment #179
kkaefer CreditAttribution: kkaefer commentedWhile 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.
Comment #180
RobLoachFixes some standard whitespace nit-picks.
I was also thinking about attached_js/css and was thinking of turning it into something like:
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:
Thoughts?
Comment #181
Bojhan CreditAttribution: Bojhan commentedkkaefer: 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.
Comment #182
kkaefer CreditAttribution: kkaefer commented@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.
Comment #183
greenSkin CreditAttribution: greenSkin commentedVery 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.
Comment #184
int CreditAttribution: int commentedComment #185
kkaefer CreditAttribution: kkaefer commentedWhile 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.
Comment #186
Frando CreditAttribution: Frando commentedI 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.
Comment #187
kkaefer CreditAttribution: kkaefer commentedThanks 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: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.Comment #188
Frando CreditAttribution: Frando commentedThanks 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.
Comment #189
webchickAwesome!! 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!
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?
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):
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.)
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?
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.
Needs to wrap at 80 chars.
Everywhere you're using #attached_js:
We now concatenate with a space before and after the dot.
Comment #190
webchickI'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:
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
Comment #191
kkaefer CreditAttribution: kkaefer commentedNo, don't look at collapse.js, that'll make your eyes bleed. Look at misc/vertical-tabs.js instead.
Comment #192
kkaefer CreditAttribution: kkaefer commentedI added more verbose comments to
#attached_*
and$element['#printed'] = TRUE
as well asform.js
. The charCode warning is not from this JavaScript file but probably from jQuery.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?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.Uh, and I actually wrote that patch…
Comment #193
kkaefer CreditAttribution: kkaefer commentedI used
__
to avoid collisions. For example, if there’sfoo[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.Comment #194
ChrisKennedy CreditAttribution: ChrisKennedy commentedLove 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.
Comment #195
quicksketchHere's a reroll that addresses all the nitpicks in #194 (a great review, btw).
This isn't necessary because the text is already translated when each module calls fieldset.setSummary()
Fixed.
Removed. I agree this brings it inline with other JS files.
Removed. You're right it's not necessary.
Now reads: " * The vertical tab object represents a single tab within a tab group."
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.
Switching this to a span with class "title" was trivial and works just as well. One less CSS rule too.
modules/upload/upload.module: here too: .'/upload.js'
Seems these were already fixed.
Fixed.
Definitely a separate issue.
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.
function() {
, notfunction () {
, which I fixed in the summary implementations for each module.Comment #196
quicksketchI 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).
Comment #197
quicksketchFinally, 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.
Comment #198
webchickWith 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.
Comment #199
xmacinfoThis 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?
Comment #200
keith.smith CreditAttribution: keith.smith commentedComment #201
yched CreditAttribution: yched commentedAbsoluetly 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.
Comment #202
yched CreditAttribution: yched commentedSame patch, fixes comment errors.
Comment #203
kkaefer CreditAttribution: kkaefer commentedThanks for the great reviews and all your help to land this patch.
However, I strongly disagree with changing the title from
strong
tospan
.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 astrong
. This patch is independent from Yves’ patch above and shouldn’t interfere with it.Comment #204
yched CreditAttribution: yched commentedMinor 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'
Comment #205
webchickOk, 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.
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.
Comment #206
yched CreditAttribution: yched commentedRerolled for webchick's remarks.
Comment #207
webchickOk, awesome. Committed #206. :)
Now onto #203...
Comment #208
webchickGrrr.
Comment #209
quicksketchI'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.
Comment #210
webchickkkaefer'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:
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!
Comment #211
yched CreditAttribution: yched commentedShould #204 be handled in a different thread ?
Comment #212
webchickYeah, sorry. I missed #204 initially. I created #431300: Authoring information vertical tab mishandles Anonymous. for it.
Comment #213
webchickAlso #431304: Summary missing on File Attachments vertical tab which hopefully is a quick fix.
Comment #214
keith.smith CreditAttribution: keith.smith commentedFirst 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.
Comment #215
webchickCommitted. Thanks, Keith! :)
Back to NW for docs again.
Comment #216
birdmanx35 CreditAttribution: birdmanx35 commentedHmm, as far as Keith's patch, I like "features automatic summaries and increased usability" better than "increases usability". Could I get more picky?
Comment #217
keith.smith CreditAttribution: keith.smith commentedYeah. 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).
Comment #219
RobLoachLet the usability begin! #439798: Vertical Tabs for the Node Type Form
Comment #220
RobLoachUpdate docs for the
#attached_js/css
:Note that we'll also have to update the Form API Reference.
Comment #222
webchickCurrent 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. :)
Comment #223
kkaefer CreditAttribution: kkaefer commented@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).
Comment #224
kkaefer CreditAttribution: kkaefer commentedFirst 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.
In this example, the fieldset named
settings
is added to the group namedmiscellaneous
. 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:
In this example, a new form element type named
pane_slider
is added in theexample_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 functiontheme_pane_slider()
wraps its sole child (the fieldset) which in turn contains the other fieldsets.Comment #225
kkaefer CreditAttribution: kkaefer commentedComment #226
Bevan CreditAttribution: Bevan commentedkkaefer. Please see http://drupal.org/node/437546#comment-1508184 with regard to documentation usage of vertical tabs.
Comment #227
RobLoachHere's the updated #attached_js/css with more examples as suggested by Konstantin.....
Comment #228
kkaefer CreditAttribution: kkaefer commented@RobLoach: looks good. Thanks!
Comment #229
Xano@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.
Comment #230
RobLoachI 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.
Comment #231
XanoWhen 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.
Comment #232
kkaefer CreditAttribution: kkaefer commentedThe 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.Comment #233
mgiffordJust tagging accessibility issues
Comment #234
neochief CreditAttribution: neochief commentedJust for your information, there's another bug fixed in 6.x #478078: "Don't create new revision" in revisions tab.
Comment #235
TheRec CreditAttribution: TheRec commentedThe example for inline attached_js in #220 is wrong :
should be
** EDIT ** I edited the "Converting 6.x modules to 7.x" page accordingly.
Comment #236
emmajane CreditAttribution: emmajane commentedThis 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!
Comment #237
Bevan CreditAttribution: Bevan commentedEmma 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.
Comment #238
emmajane CreditAttribution: emmajane commented@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. :)
Comment #239
Bevan CreditAttribution: Bevan commentedEmma 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:
Comment #240
Bojhan CreditAttribution: Bojhan commentedI will work on this documentation.
Comment #241
Everett Zufelt CreditAttribution: Everett Zufelt commentedWas 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?
Comment #242
Bojhan CreditAttribution: Bojhan commentedYes 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.
Comment #243
yoroy CreditAttribution: yoroy commentedEverett, 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 :-)
Comment #244
Everett Zufelt CreditAttribution: Everett Zufelt commented@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
Comment #245
mgiffordAlso see this issue for keyboard only users:
#567390: Use vertical tabs with keyboard only
Comment #246
Cliff CreditAttribution: Cliff commentedSubscribing. 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.
Comment #247
redben CreditAttribution: redben commentedRegarding 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 !
Comment #248
alexanderpas CreditAttribution: alexanderpas commented@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.
Comment #249
Cliff CreditAttribution: Cliff commented@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.
Comment #250
redben CreditAttribution: redben commented@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 !
Comment #251
moshe weitzman CreditAttribution: moshe weitzman commentedI think this tag was removed by accident. I can't find any docs.
Comment #252
dddave CreditAttribution: dddave commentedNo accident about removing the docs tag. See #242.
I leave it in case Bohjan was held up.
Comment #253
matt2000 CreditAttribution: matt2000 commentedDisclaimer: 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.
Comment #254
Bevan CreditAttribution: Bevan commentedAll 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.
Comment #255
Les LimI'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.
Comment #256
Bevan CreditAttribution: Bevan commentedMoshe (#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()
andform_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).Comment #257
catch#653068: Update documentation is incomplete
Comment #258
sunThe 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.
Comment #260
yched CreditAttribution: yched commentedFollowup : #857124: Collapsible fieldsets and vertical tabs do not work without form_builder() (Form API)