[UX] Add parent menu selector to menu add/edit link form

[bugfolder]

Description of the need

Currently, when you add a link to a menu via /admin/structure/menu/manage/menu_name/add?langcode=und, you choose the parent link via a drop-down selector, whose options include every possible parent link, including links in other menus.

Since you've just gone to this page to add/edit a link to/in this specific menu, it seems a bit odd to list links from all the other menus, too. "If I wanted to add a link to a different menu, I would have used 'add link' for that menu."

But there's a bigger problem if there are many menus; it's easy to get lost in the giant list when you're scrolling through a big menu. The admin bar alone adds lots of items to the select menu. For example, here's what the drop-down looks like when you're editing a link in the Account menu, which contains only two items:

On one of my sites, the organization has one or two events a year, and we create a new menu for each event for visitors to navigate through that event's pages (anywhere from 5 to 40 pages, i.e., 5 to 40 new menu links per event). And we don't throw away past events. So after several years, we've built up about 25 menus, and something like 2000 menu links in that selector to contend with when we're building up the next menu. It's very easy to get lost (especially since many of the menu links have similar names from year to year).

To deal with this, I wrote a small custom module that adds a separate selector for the "Parent menu" to the menu add/edit link form, so that admins can first select the menu, and then the "Parent link" selector only displays candidate parent links from that particular menu. It looks like this:

It was pretty simple to implement: a hook_form_FORM_ID_alter() instantiation, plus an ajax callback to update the "Parent link" selector when the "Parent menu" is changed.

It occurs to me that this might be desirable default behavior for these two forms for others who don't want or need to see every possible menu link in the drop-down when they're trying to add or edit a link in a specific menu.

There's a couple of ways I could proceed from here. I'd like to get some feedback from people on the possibilities.

1. Submit a PR to change the core behavior to add the "Parent menu" selector for everyone.

2. Offer it as an option in core, via a "Menu settings" configuration checkbox (at admin/structure/menu/settings). That way the default behavior wouldn't change, but people could turn it on (at least, if they know to go look for the setting).

3. Don't change core, just make it a contrib module. (Currently it's custom.)

I'm happy to do any of the above, but there's two drawbacks to making it a contrib module. First, it's a lot less discoverable. Second, in a contrib module, the alteration happens via a hook_*_alter(), so after the original form is fully built, which means that one is still constructing the list of all possible menu links before replacing it with the more narrowly targeted list—so, a bit inefficient.

What do people think? Is this worth putting into core? If folks are in favor, I'm happy to provide the PR.

[olafgrabienski]

Great idea! Using already (too) many menu related contrib modules, I'm in favor of a core integration. Personally, I'd prefer the new feature as an option.

That said, I'm wondering if the dialog becomes potentially confusing when people see two options with the same default value and similar labels, like this:

Parent menu User menu

Parent link User menu

First idea to improve the dialog: Call the "Parent menu" label just "Menu".

[bugfolder]

I'm in favor of a core integration. Personally, I'd prefer the new feature as an option.

I've created a PR that implements it as an option (set on the admin/structure/menu/settings page). Here's an image of what it looks like when turned on. A few things to note:

• To avoid the weirdness of the default choice for link looking exactly like the menu name above, when this option is turned on, I add "(top level)" to the link selector to provide some differentiation.
• To make the setting discoverable, I've added a "Configure this selector" link to the description, which links to the settings page.
• There's an interesting side effect for the weight, whose range comes from the range of menu item weights. That control will switch between a number field (with an up/down mini-button) and a selection, depending on how large the range is (large ranges = number field, small ranges = selector). That was always the behavior, but previously, the range was always the range over all possible menus, and the gigantic Admininstration menu always blew it into number-field territory. Now, if you pick a small menu (like the Account menu), you'll get a selector.

@olafgrabienski, @argiepiano, please take a look and give me your suggestions! (And of course, anyone else.)

[klonos]

Thanks for starting this issue and for contributing the solution that has worked for you @bugfolder 🙏🏼 ...I also agree that this should be in core, but I'd like it to be the default and only option. I don't see how the previous way of dumping all menus and menu items in the selector makes sense when you have already selected which menu you'd like the menu item to be added to. Basically what you already said with this:

... it's easy to get lost in the giant list when you're scrolling through a big menu. The admin bar alone adds lots of items to the select menu. ...

This solution here solves the UX problem, and I don't see why we should allow the problematic behavior + also add UI complexities by introducing an option for it. I say remove the option - make this behavior the default, and call it a day.

I've left a couple of questions/suggestions in the PR, but overall this is looking very good. Very few code changes, for a rather considerable UX gain 👍🏼

[klonos]

...also, can we please extend the scope of this to also add the same behavior in the "Menu settings" vertical tab of content types as well as the content edit form? (when more than one menu has been configured to be available for a content type)

[bugfolder]

I'd like it to be the default and only option....

I personally would also lean this way, but we've received at least one other vote for "make it an option" (@olafgrabienski) so it would be nice to get further input from others to establish consensus (or at least, majority).

Thanks for your review and comments! I've implemented one, gave reason for another, and queried back about a third. Of course if we make it the default/only behavior, I'll strip out the setting.

[bugfolder]

...also, can we please extend the scope of this to also add the same behavior in the "Menu settings" vertical tab of content types as well as the content edit form? (when more than one menu has been configured to be available for a content type)

It looks like the vertical tab of content types already has this behavior (or perhaps I'm misunderstanding what you're talking about). Here's the content type edit page with two menus selected, and only those two menus' items are being shown in the default parent link:

And here's the content edit form, which shows the same thing:

[olafgrabienski]

I'd like it to be the default and only option....

I personally would also lean this way, but we've received at least one other vote for "make it an option"

Prefering the feature as an option was probably result of my first concern that the dialog could become confusing. Now, where this has already been adressed, I'm fine with the new feature as only behavior.

[bugfolder]

Option is removed. That also allowed tightening the code a bit, and I fixed a minor bug I'd overlooked.

[bugfolder]

Oops, lost a test case. Probably need to update the tests.

[bugfolder]

Tests fixed and running successfully. Ready for further testing/review (but see my answer/further query to your question in the PR, @klonos).

[klonos]

...or perhaps I'm misunderstanding what you're talking about ...

Yup, that 😅 ...perhaps I didn't provide enough information to make it clear. If you enable all checkboxes for all available menus, including the admin menu, then the parent menu selection becomes unwieldy again.

So OK, allowing content links to be added to the admin bar is an edge case, however I used the admin menu as an out-of-the-box example which is easy to test. You could generate a fresh, rather huge menu with devel (or a few smaller menus and enable all of them). Then the problem we are trying to solve here is present in the content type configuration form, as well as the content edit form.

Prefering the feature as an option was probably result of my first concern that the dialog could become confusing. Now, where this has already been adressed, I'm fine with the new feature as only behavior.

Option is removed.

Nice! ...code looks much cleaner now too 🙂 🙏🏼

I've replied in the PR.

[bugfolder]

Yup, that 😅 ...perhaps I didn't provide enough information to make it clear. If you enable all checkboxes for all available menus, including the admin menu, then the parent menu selection becomes unwieldy again.

OK, that's feeling out of scope for this issue (which deals with just the menu add/edit link form), and at least a little more discussion is needed. I"ll start a new issue and copy over the relevant bits of discussion.

[klonos]

Sure 👍🏼 ...I was just thinking/hoping that we could perhaps abstract some of the logic, since we will be doing the same thing. But I'm fine with the follow-up issue.

[bugfolder]

Working on #6392, I've discovered some things that could be improved here, so setting it back to "needs work" for the moment.

[bugfolder]

I've simplied the ajax, and also moved the appending of "(top level)" into the helper function so that other similar implementations can make use of it (like #6392). @klonos, I left a question back for you in the PR about replacing '#prefix' => '<div id=...>'with a '#type' => 'container' wrapper.

[klonos]

@bugfolder looks good, and I have replied to your question 👍🏼 ...in short, I still think that we shouldn't be using '#prefix' that way if it can be avoided, and have suggested to consider using #wrapper_attributes instead?

[argiepiano]

A sidenote observation to the #prefix / #suffix discussion: changing this by using a wrapper changes the array structure of the $form variable, which may break existing form alter hooks in contrib (or in custom modules created by site builders). This was pointed out by @bugfolder and I think it's a valid concern. While I agree that #prefix ... should be avoided, it's not a good idea to make this change in existing code that already uses it.

Although, mitigating this concern is the fact that the PR does make changes to the array structure even before changing the #prefix use...

[bugfolder]

mitigating this concern is the fact that the PR does make changes to the array structure even before changing the #prefix use...

Yes. It's a little unfortunate. The most efficient way to implement the new behavior is to put everything that changes inside of a single '#type' => 'container' wrapper, and then the ajax can use the simplest 'callback'/'wrapper' idiom. But, as you observe, that changes the $form array structure.

Safest would presumably be to preserve the existing $form elements and just add new elements or properties where needed. I'd done that in an earlier version of this PR, and it looks like I should go back to that way of doing things.

Even replacing the existing '#prefix'/'#suffix' with '#wrapper_attributes' in $form['parent'] to get the id in place changes the HTML, so potentially breaks themes (the selector #menu-parent-select-wrapper .form-item-parent would no longer work, for example, since both end up in the same div with #wrapper_attributes). But we can use '#wrapper_attributes' on the new bits.

[klonos]

Right 🤔 😞 ...I guess we'll need to keep doing what I personally consider a hack for now, and I'll raise an issue to clean things up in Backdrop 2.x then 🤷🏼

Thank you both for the feedback/insight @bugfolder and @argiepiano 🙏🏼

[stpaultim]

We looked at this issue/PR in the Design/UX meeting last week. The change looks good to me. Thanks for the work going into this issue.

[indigoxela]

As with all form structures in core, that change - we have to thoroughly consider backwards compatibility.

Both, hook_form_alter() and hook_form_FORM_ID_alter() implementations may be affected. We don't want to break things.

[indigoxela]

To revive this issue... We'd need a little more testing re form structure change and possible impact on existing contrib modules. Probably it's nothing, but better safe than sorry. :wink:

Several modules implement alter hooks on the menu_edit_item form. Let's make sure, they don't break. Here's a quick list of implementations, for further recherche / testing: https://github.com/search?q=org%3Abackdrop-contrib%20menu_edit_item&type=code

Additionally it seems, like the removal of function menu_update_parent_options_ajax() is premature, as it's still in use elsewhere.

[olafgrabienski]

Several modules implement alter hooks on the menu_edit_item form. Let's make sure, they don't break.

I'm using some of those modules, but I'm not sure what to look for to test them. Or does it however make more sense to ask the module maintainers?

[izmeez]

@olafgrabienski Can you just use the PR to patch core and see if it causes any problems with the modules you are using?

[olafgrabienski]

@izmeez That could work somehow on real world sites: sooner or later some issues might come up, or not. I'd prefer however to test with more specific ideas what to look for.

[stpaultim]

In Zulip @indigoxela asked why this one is stalled. From my review of the issue, it seems like momentum was stalled by this concern:

To revive this issue... We'd need a little more testing re form structure change and possible impact on existing contrib modules. Probably it's nothing, but better safe than sorry. 😉 https://github.com/backdrop/backdrop-issues/issues/6390#issuecomment-2254027487

I've put this on the agenda for the next dev meeting to discuss whether or not the potential conflict with contrib is an issue and IF SO, how to get past it.

Any thoughts?

[indigoxela]

Any thoughts?

Testing would be a huge help. Whether there is even a conflict with one of the contrib modules helps a lot for a decision, I guess. And I mean testing with the contrib modules, that implement an alter hook - the link in my previous comment should help to identify them.

And, of course, function menu_update_parent_options_ajax() should not get removed, yet - it's still in use in core. And the conflict should get resolved (update and rebase the PR). Local testing's tricky, as the patch won't apply. :wink:

[stpaultim]

This was discussed during meeting today starting at: https://www.youtube.com/live/O17yLvRozoM?si=7ROrs9HXd1oEN79d&t=2674

@quicksketch Did not think that this poses a significant risk to Backwards compatibility in Contrib. But, additional testing would still be useful. I've been a little unsure of what to test or how to test it most effeciently, but I suppose I could just try applying this to some sites with "menu" related contrib modules. I'll try to do that this weekend.

I believe that there is also a comment on the PR that is unresolved.

@bugfolder Any chance that you will have time this weekend to update this PR so that sandbox works?

[bugfolder]

Any chance that you will have time this weekend to update this PR so that sandbox works?

Pretty tied up these days, but I'll try to put some time on it.

[stpaultim]

I'm going to rebase this PR, just so that we can have a sandbox and folks can test it.