UB Usability: menu parent choosers

I'm getting on the wagon to assist with this UI enhancement. To describe it in a way that I understood:

- Go to admin/build/menu/item/22/edit on a new D6 install. The drop-down select for parent contains the entire menu tree.
- Such a large tree is a huge scalability problem.
- We should replace this with a drill-down type of solution of a select list for each parent. Selecting a parent creates another select list of all that item's children.

Probably the best news of all of this is that no new javascript will be necessary (I hope) thanks to our new ahah.js file and javascript behaviors. We'll just need to make a menu callback and setup the #ahah properties.

I've made the deal that if chx (or anyone) writes the non-js version with an update button to update parents, I'll write the AHAH implementation.

Yes, I told quicksketch that I will write the chooser because this is a usability and a scalability bug. Try to pick a book parent right here on this site...

Note that I have not yet this to CNR because the parents are not yet saved and I am not satisfied with the wiring in of ['menu']['parents'. I will investigate more but, release early, release often, so here it is.

Though the node form element and the book needs to conversion, the menu item edit form works. Still you need #tree TRUE, this bothers me a bit. I will see what I can do.

So, if you have a widget like this where a button needs to act on other elements then put those elements in a wrapper and point a reference to that wrapper as a custom property of the button. Because you have $form_state['clicked_button'] and you have a reference in there, it's surprisingly easy. It's even shorter than the #tree code but more versatile. Oh joy.

If some JS guru adds AHAH and adds some JS which changes the select size from 1 to 5-8 and finally moves the selects besides, then do you know what we will get? iTunes.

You could call me the JS guru...
I have it all figured out, but I'm still running into problems with locating the form element...
SO...
I have one big code comment for chx to read when he gets back from his sleep

the status box seems not to work
/me files a bug

I took a stab at this based on what poll does. You have not added the style changes discussed and the Update button does not disappear.

Here's a patch that has the ahah request being made properly. Note that it adds a theme function so be sure to clear your cache.

Here's another patch which takes the Update button out of the parents array and puts it into a separate wrapper element. Chx informed me the previous method was a bad idea, causing a recursion error.

Try editing a menu item :D book and node edit is coming. Also, the patch depends on http://drupal.org/node/193191#comment-633100 .

OK, now saving works based on http://drupal.org/node/193191#comment-633154 . As some of the burden is now on form.inc , the patch is smaller and a bit simpler.

aclight complained about --- not working. Fixed. Added it to the first box so you can choose root.

I tested this on FF 2.0.0.9 on WinXP, IE6 on WinXP, and IE7 on Vista and it works well. I think this is a big improvement over the select list that can get very long.

Two minor problems:

1. On IE6, occasionally when I clicked on one of the items in one of the children boxes, that box and all children boxes of that box (if any) would all turn gray, and I needed to click one of the items in the parent box to "unfreeze" things.

2. In both IE6 and IE7, when I select an item in the child box, the scroll position in that box is shifted such that the selected item is at the top of the visible items in the list. This is a bit inconvenient since, to select the "-----" item, I then have to scroll up in the box. It would be nice if the scroll position did not change, which is the case in FF.

Opera and FF2 on Linux both work very well. The scroll issue is hardly a real/fixable issue, that's how IE behaves, I do not think we need to bother too much. That freezing thing... I do not know. it's minor and occasionally... so... if Nate can catch the bug, the we will fix otherwise, I say, it's OK.

Did some quick testing:
-The UI shouldn't display parents that will result in the menu becoming too deep.
-I'd prefer to see appropriately indented drop down selects on separate lines, rather than the side by side scrolling selection boxes.

I might not have understood the issue cleanly, but the issue cried for a better title. So if I did not understand well, please retitle and explain.

I added menu select as the first box and fixed depth problems. Reviewers: unless http://drupal.org/node/193191#comment-634164 is committed by the time you review this, you need that, and don't forget the menu rebuild. Also, only review menu item edit/add, because only that works, with more coming.

Bug report:
I went to page http://localhost/d6/admin/build/menu/item/9/edit on a fresh d6 install. Then I applied this patch and http://drupal.org/node/193191#comment-634164 and reloaded the page. Then I clicked "parent" and saw the two select boxes, one after another. I clicked "My Account" in the first box. The page did an AHAH update, moving the two select boxes to be side-by-side, and the attached error box popped up.

Ah, I just needed to rebuild my menus, by going to admin/build/themes and saving.

Works for me. Slick! I still hope Nate's drag & drop menu reparenting gets in, but dynamic subselects are a fantastic widget for drupal to have.

I get at "Fatal error: Call to undefined function menu_parent_options() in W:\www\drupal-cvs\modules\menu\menu.module on line 507" after applying this patch and visiting node/add/story, or any other content type.

Can't wait to let http://drupal.org/project/hierarchical_select take advantage of the same techniques!

I've found a bug: if JS is disabled, and you click the "Update" button, then the text fields you've just filled out are emptied.

Also, there's a "/* RTL */" comment in the LTR CSS instead of the RTL CSS :P

Reviewers: don't forget the menu rebuild. Also, only review menu item edit/add, because only that works, with more coming. fajerstarter skipped the second part of this note. starbow skipped the first part.

OK fresh install, patched before installing then enabled all contrib modules.

Very, very nice. The form refresh on firefox was a little slow, but not too much (and my connection has been terrible today so could be my end). So much better than the huge select list and this will be great for all kinds of stuff. Saving seemed to work fine etc.

Tested in IE7/FF2.

Thought I'd put together a 60-second video of this.
http://blip.tv/file/get/Kentbye_tech-MenuParentSelectors263.mov

Played around with it a bit on FF2.0.0.9 and it looks good.

I like it.

How about moving Weight and Expanded to the Parents fieldset?

In the video, why is the title of the fieldset 'Parents'? Can you select multiple parents or just one? Shouldn't it be 'Parent' (singular)?

Why is the parents fieldset the only collapsed fieldset? That looks inconsistent with the rest of the form.

Why is the status message shown inside the fieldset? That is also inconsistent with the rest of Drupal.

If we're going to commit more usability improvements before tomorrow night, I'd suggest we focus on (i) the menu module DnD patch, (ii) the taxonomy DnD patch and (iii) the book module DnD patch. Let's tackle the other ones first so we don't risk losing our focus. If the other patched landed, we can look at this one. If not, it will have to wait for D7. Thanks.

Dries, although this is also about usability, I think chx is rightly concerned about potentially many thousands of menu items being loaded into memory all at once. Although I'm a big fan of those other three patches they don't have the (potentially quite bad) scalability issues that this one deals with. Edit: and menu drag and drop is in! so it's just book and taxonomy left now.

Marking to needs work for the fieldset issues as identified in #29 and #30 and I'll go review drag and drop patches now!

subscribing - thanks chx for pointing me to this

I hear and obey. With a heavy heart, I say, let's get the drilldown element into Drupal 7. But, we must have a way to override the default selectors or all our work to keep D6 menu scalable was in vain. This patch is the absolute, bare minimum I could come up with. After commit, please reset to 7.x-dev , code need work. Thanks.

Do not get me wrong, I will post the menu parent patch as soon as it's ready which surely can't be more than a few hours away. But I am not risking all the hard work we did on a complex patch.

Well, Dries' concerns in http://drupal.org/node/191360#comment-635000 does not look like unsolvable within this timeframe. Why not look into them?

Gabor, the patch above and what I have currently does not work with node form and I am not 100% I can fix it. If I can, great, if not, well, the fallback patch is there. I am trying :)

The node form needs more consideration than what little I can give it now. if it can get in past beta3 , good but if not, then let's get the plugin version in and see what we can come up in contrib and in D7.

@Dries: "Why is the status message shown inside the fieldset? That is also inconsistent with the rest of Drupal."
I believe was my fault in the production of the screencast.
I saved the theme form, but then shut the window before the status message could be displayed because it would've been 7-10 seconds of dead time.
When the theme page was saved, then the status message was incorrectly displayed on that page.
An edge mistake IMHO, and shouldn't reflect negatively on this patch.

chx: Understood. So why not add some docs to this override? :)

From menu_parent_options

The menu_links table can be practically any size and we need a way to allow contrib modules to provide more scalable pattern choosers. hook_form_alter is too late in itself because all the possible parents are retrieved here, unless menu_override_parent_selector is set to TRUE.

OK, committed this for now. Happy hacking in D6 contrib :)

Still only for menu items but the depth is working and also it's totally abstracted out: there is a drilldown eleement with process helpers in form.inc, the appropriate AHAH callback is now in system.pages.inc , the menu parent creator is in menu.inc so it can be reused in book and menu modules both. The depth limiter works. We are quite prepared fior node because now we do not run our submit instead we run a process. There is a CSS glitch, for some reason Weight also floats despite it's definitely out of the div.

minor UI point: the weight select should be above or below the drill-down selectors. Now it's appearing to the right of the selects and is bumped around when changing selections (at least on FF 2 on linux).

In the code I'm not clear why you are adding a new possible -1 setting to the 'customized' attribute of menu links?

Also, why in the _menu_parent_depth_limit function that moved to menu.inc is the check for 'has_children' omitted compared to the original - seems a cheap check to make to avoid a function call and query.

Also, since you're moving _menu_parent_select_recurse to menu.inc, is the plan to try to re-use this function when doing a drill-down for book module?

Also, I don't know much about JS, but it looks as though you are submitting the entire form every time there is a change? Is this a better approach than rebuilding the form in the cache, as is being done now for the book parent selector? Seems like it would be much slower?

As said, I was quite prepared for the node form. The only thing missing was the node_form itself -- you see, this is not included by default so we need to store this fact. I have found a nice unused piece: cache headers. I store there and pass around the router_item_file in form_state. book is coming tomorrow. (Yes, preview keeps the picked menu parent, this is why we are not using submit but process.)

Forgot to react to pwolanin:

• minor UI point: the weight select should be above or below the drill-down selectors -- yes, i noted this in the previous comment, added a clear-block class to the wrapper div on catch's advice, works.
• In the code I'm not clear why you are adding a new possible -1 setting to the 'customized' attribute of menu links? -- another thing I forgot to mention: this patch currently includes momentarily http://drupal.org/node/194166 otherwise things do not work
• Also, why in the _menu_parent_depth_limit function that moved to menu.inc is the check for 'has_children' omitted -- because has_children is not provided by menu module in #item. Given that this runs infrequently, I think we are fine.
• when doing a drill-down for book module -- book module will get the exact same chooser.
• submitting the entire form every time there is a change? Is this a better approach than rebuilding the form in the cache, as is being done now for the book parent selector? Seems like it would be much slower? -- we have no idea where this element is in the form tree thus we can't rebuild just this element. Tricky optimizations are accepted later.

It's going to be a bit hard to re-use this for books, unless you split them apart from the existing book chooser - this contains options like "create a new book" and "none" which don't exist in the context of the menu module.

I tested the patch in Safari and it looked good.
Here's is an updated video:
http://blip.tv/file/get/Kentbye_tech-Menu_Parent_Selectors_V2_476.mov

I had some weirdness in FF2.0.0.9 where the new child options were appearing below the parent instead of next to it.
I'll try to reproduce & capture it.
UPDATE: I was not able to reproduce the FF glitch.
Worked fine as well.

I am resetting to review to get reviewers. We shall see what the book chooser will be, but just because of that, I am not setting this to CNW. I would like to get reviews. Also, I do plan to replace totally the current chooser. Again, we shall see. Kentbye, thanks for the video!

OK quick review in FF2 and IE7, this is looking good.

In FF2 I get the scrollbars regardless of if there's a need to scroll or not. In IE7 they appear depending on the number of items, which is nicer.

In both, it's very, very quick.

Only other thing I noticed was if you set menu options before entering a title in the node form, you get the "you must enter a title" validation error, that's not a big issue but it was a bit of a jolt considering the form hasn't been submitted or previewed by that point.

I can remove displaying the errors, that's very easy. Is that what we want?

It's pretty common to fill different bits of the form out at different times, so +1 for me for suppressing errors until a full preview/save.

While I like where this is going, I don't like the way the new menu lists appear on the right. On a small screen resolution, this behaviour becomes a little tricky as bits start falling into a new line.

I would recommend something in the line of sliding menus. It allows greater control over the width without having elements fall due to screen size. dvessel's excellent lumen theme highly demonstrates this concept:
http://si-lumen.us

There was talk of Greggles ripping this out into a separate module but I'm not sure if this materialised:
http://groups.drupal.org/node/2839

I am against holding this up, but would be good if we can get the current functionality in for Beta 3, and then reopen the issue for UI improvements?

This is very, very close -- we might consider committing it. The book is almost ready and it'll be able to use this code and I will upload that soon, too but if this gets in that's already a big boon. I have upgraded the code to work with any number of drilldown elements,previously they all worked into the same form_state['drilldown'].

ok - selector seems to be working well on the menu item editing form except for a very minor complaint that if I have scrolled the select, the newly selected item jumps to the bottom of the select when the selector repopulates.

After saving the item on a page like admin/build/menu/item/362/edit, I did not get properly redirected to the menu overview. Instead I go back to the edit form, and the data in the select is stale - it shows the parent before editing started. However- I wonder if this is a bug in the AHAH JS?

The selector in the node form doesn't seem to work - it doesn't appear. If I submit the node form I get: notice: Undefined index: select in /Users/Shared/www/drupal6/modules/menu/menu.module on line 351.

Note: testing node with FF 2.0 on Max 10.4

Problems fixed, they were really minor. Sorry for posting w/o much testing, I was in a great haste, I have little time left before beta4 and now I missed a perfect opportunity to get an RTBC :(

chx, how should one test this??

admin/build/menu/item/%/edit is flawless, very speedy, very easy to use.

It's not saving the menu items from the node form for me though - they don't show up when I edit the node again, nor in the menu admin screen.

Stefan: it'll make sense when you see it, just remember to trigger a menu rebuild otherwise you'll get all kinds of craziness :)

I just double checked and it does save -- if you have specified a menu link title. Seeing Drupal providing no feedback if you forgot, I added an error message to warn you. Yes, book is still not in the patch but there is a beta4 coming and I would like to see this in. Book is tantalizingly close but I would rather not miss beta4 because of that.

The only difference to #57 is the new error message and the omission of the not-yet-working book changes.

chx is right, I wasn't specifying a title. This all works very smoothly.

The only niggle I have is that with js disabled, the "update menu parent" item is inline with the select boxes, it should really be above or below. Maybe take it out of from['wrapper']?

Leaving at RTBC anyway since this that shouldn't hold this patch up.

This is all possible in contribs now with the extension points we added earlier to Drupal 6, right? I would not like to introduce a new UI concept again (with people rushing to find bazillion places to apply it in Drupal), in part because

- we are not covering these bazillion places in Drupal (inconsistent UI for the same tasks)
- we introduce a new UI concept which was debated above with its unfriendliness to not so wide layouts
- we make changes to the ever changing menu code again
- as said, you can do this in contrib now

Please understand that we try to concentrate on stabilizing and fixing bugs in Drupal 6, rather then adding new stuff to it. You are in a unique good position that these changes are doable in contrib for Drupal 6, and can be introduced in Drupal 7, with the UI concepts refined through the Drupal 6 lifetime. Thanks for all your work.

I hope someone else takes this from here.

http://drupal.org/node/203010 was duplicate.

subscribe

We still need this, but it no longer applies.

I reckon something like this is the most likely option for removing some of the confusion around 'parent item' (and the menu fieldset in general).

At the moment, we have a long list of almost undifferentiated links in a select - with this, we'd clearly display the top level menu, then children programmatically - and it might be enough to put "select menu position" and avoid parent/child language completely.

So, bump.

Note: Hierarchical Select 3 supports this. It will be ported to Drupal 6 very soon. But, in its current state, it cannot be accepted into Drupal core, it will have to be refactored. In any case: if you want to improve the usability of this, you can, for both Drupal 5 (today) and Drupal 6 (soon).

Second note: this issue also implies fixing a scalability issue: if the menu tree is huge, it'll work very slowly in its current state.

I'd like to see a similar method for choosing taxonomy parents, so something centralised (like hierarchical select) woudl be good for that - haven't looked at it for a while though.

Taxonomy is already supported! But now you mention it, I haven't overridden the form for creating/editing terms yet. Will do that later today.

EDIT: forgot to add this: I'm currently adding support for selecting Book page parents, too!

Wim Leers - I know it's in hierarchical select, I meant getting a comprehensive solution for multiple-depth select forms into core - so we can have a unified UI between menus and taxonomy (and maybe book module on the node form as well).

I know I know, I'm only indicating that it's already supporting every place in core where it can be used :)

Subscribe. This "comprehensive solution for multiple-depth select forms" is an excellent example of a useful new tool in the Drupal UI toolkit.
Let's see what we can do about this @Szeged Drupalcon.

I wish I'd seen this thread earlier - then I would have understood the history behind the strange 'extension stub' which was all that made it into D6 for hierarchical parent selection. I've almost finished porting the Category module to D6, and I've borrowed the AHAH parent-select from book.module for it - would have been better to have implemented something more fancy.

I'll be keeping my eye on Hierarhical Select, and hopefully helping to get it ported to D6. Would be great to get all or part of it into core for 7.

Subscribe.