Make menu trail behaviour in SystemMenuBlock optional

MR 12375 is up.
I added tests for the active trail functionality, but I didn't do one for the #states handling in the UI. Is that needed?

Also, I wonder if there needs to be either help text in the description or more conditional logic so that the new setting isn't applied when the start level is configured to be greater than 1? Effectively it creates an empty menu if the start level is greater than 1, the menu is set to expand all items, and ignoring the active trail is enabled.

    // For menu blocks with start level greater than 1, only show menu items
    // from the current active trail. Adjust the root according to the current
    // position in the menu in order to determine if we can show the subtree.

Also, I wonder if there needs to be either help text in the description or more conditional logic so that the new setting isn't applied when the start level is configured to be greater than 1

We might need both #states and config validation for that?

Added more #states handling and the config schema validation constraint.
Also added functionaljavascript test for the form.

Only thing I'm wondering here is whether instead of the negative we could use 'Use active trail' or similar.

Then the #states could only show the levels etc. options if it's checked, otherwise hidden. However that would complicate the upgrade path because we'd need to set it for existing config using those options to prevent them being hidden in the form when it's edited.

Yeah, I went with the negative to make the upgrade path easier (NULL being the same as FALSE), despite that it makes the wording maybe a bit awkward.

Though maybe setting the value of the new property in defaultConfiguration() to TRUE makes that concern go away?

Then the #states could only show the levels etc. options if it's checked, otherwise hidden. However that would complicate the upgrade path because we'd need to set it for existing config using those options to prevent them being hidden in the form when it's edited.

Thought about this some more. If we have the new "track active trail" (wording TBD) field control the level and expand_all_items, and assuming "level" is set to 2, or expand_all_items unchecked, then when going from "track active trail" checked to unchecked, those fields would be hidden. And the result might be a bit unexpected to the user, even if help text is provided. We'd probably also have to disable those fields when "track active trail" is unchecked, because #states can't change field values. Then in the form submit handler, we'd have to interpret those NULL values as level = 1 and expand_all_items as TRUE.

Though maybe setting the value of the new property in defaultConfiguration() to TRUE makes that concern go away?

As in NULL and TRUE would be treated the same? I don't remember doing this but it seems possible. However:

Thought about this some more. If we have the new "track active trail" (wording TBD) field control the level and expand_all_items, and assuming "level" is set to 2, or expand_all_items unchecked, then when going from "track active trail" checked to unchecked, those fields would be hidden. And the result might be a bit unexpected to the user, even if help text is provided.

Yeah it starts getting complicated, good to think about how much work it would be before doing all the work.

Part of the problem might be the 'active trail' wording which is really an arcane internal menu system concept.

What about this for the checkbox label:

Show the same menu markup on every page

With description:

When this is checked, the menu will have the same markup on every page. Otherwise the current page's position in the menu structure will be calculated and an 'active' class added to relevant menu links if it is found.

Made the text changes and pushed. Thinking about this now, though:

When this is checked, the menu will have the same markup on every page. Otherwise the current page's position in the menu structure will be calculated and an 'active' class added to relevant menu links if it is found.

The class added in the core theme templates is menu-item--active-trail. There's also an is-active class added to a menu link if it is the current page, but that is added through JS. Still I wonder if "active" is not informative enough? Also, I'm not sure what the antecedent to the final "it" in the description text is.

My attempt/suggestion for an edit:

When this is checked, the menu will have the same markup on every page. Otherwise, the current page's position in the menu structure will be calculated and an "active-trail" class added to menu links in the current page's menu hierarchy.

That looks good. I couldn't think of a good wording for 'the current page may or may not be in the active trail' but ""active-trail" class added to menu links in the current page's menu hierarchy." covers it implicitly really well.

MR updated per #12.

This looks great to me now.

The last remaining question for me is the nullable config. It avoids having to have an upgrade path, but it means saving menu blocks in the UI will gradually introduce the new config key over time. The other option would be to make it not nullable, and have a config entity update to add the default everywhere, as well as a hook_entity_presave() for exported config.

After discovering 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active I think this is probably the way we should start doing things though - e.g. allow a gradual update over time with runtime code compatibility.

Yes, the issue with LB overrides mentioned in 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active , among others, is why I wanted to avoid doing an upgrade path. I think it's okay to have the config key introduced over time on block saves?

For block entities specifically, I wonder if we can change get()<code> so that calling <code>get('settings') will merge in configuration from the plugin collection. That way additions in defaultConfiguration() from the plugin will be picked up in the loaded block entity. That doesn't really change anything here, but it could maybe apply to other situations?

The issue with this is how often do these blocks get resaved?

The issue with this is how often do these blocks get resaved?

Here, it doesn't much matter because NULL/undefined is the same as FALSE for the new property, and FALSE is always a valid value.

It'd be an issue for any new block property that doesn't work like that. What I suggested in #15 could help address that, but if there are constraints where the default value is not valid with existing configuration, that would be another problem.

I was more concerned about how long we'd need to handle the missing config.

After looking at the actual code it's not really a huge concern.

@catch asked my opinion on the nullable approach. I think it is a good one because the new key is not added during a config import so you're not getting unexpected changes. The one thing that gives me pause is how are we going to tell any modules or recipes that supply this config that they need to update. I.e. will we leave tis key as optional...

@alexpott I think we'd need to leave the config key as optional permanently if nothing happens after this issue. We could open a follow-up to make it non-optional and try to implement that using the to-be-decided APIs in 🌱 [meta] Just in time updates Active and 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active though.

Updated MR per feedback and adjusted tests.

Updated the IS proposed resolution to match the work done so far.

Could we update the some before/after screenshots of the new config form. May need UX sign off.

Think it would need a CR?

CR: https://www.drupal.org/node/3533514 →

Note: https://www.drupal.org/project/menu_trail_by_path → adds this as a third party setting per menu as opposed to the block, so the context is still there, but by disabling it for a menu link the footer it will always have the same empty value and as a result, not vary.

This overlaps and you can then end up with a somewhat confusing situation where you enable it on the menu. Or the other way round, where it's enabled on the block but actually disabled on the menu. That means the block can force disabling it but can't force enable it-

I think that's OK and it's probably not worth to reconsider and move to the menu (and support more than one state).

I haven't tested it but my understanding of doing it at the menu level would mean it would also prevent modules like menu_breadcrumb from working, where it's fine for the breadcrumb to vary by active trail because that's what you want there. So having it at the block level means you can combine a relatively static menu while also using that structure to determine breadcrumbs.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2025-07-25 Active . That issue will have a link to a recording of the meeting. (See the second half of the recording.)

For the record, the attendees at the usability meeting were benjifisher, rkoller, and simohell. I am adding issue credit for them.

We all agreed that the option should be reversed. The current label (title), "Show the same menu markup on every page", does not give enough information, so the user has to read the description (help text) to figure out what it means. It is sort of like trying to describe what an option does not do instead of describing what it does.

We did not have time to recommend replacement text, but start with something like

Add the active-trail class

Maybe just use that and keep further explanation in the help text. In the help text, mention the pros and cons: the theme may style the active-trail class, but it has a performance impact.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

This issue reminds me of some work I did several years ago. I even gave a few presentations on it. (The most recent is https://slides.benjifisher.info/caching-midcamp-2020.html#/strategy. I have to do something about the TLS certificate.) At the time, it did not occur to me to make the same change in Drupal core.

Why not use JavaScript to add the active-trail class? It is a lot simpler: JavaScript is really good at traversing the DOM, and the current PHP code is a little messy.

But code simplification is just a side benefit. The real advantage is that you can cache the navigation and still have the active-trail class.

benjifisher → credited rkoller → .

benjifisher → credited simohell → .

We add the 'active' class in JavaScript, which is based on 'is the link pointing to the actual page you're on', but we can't easily do that with the active trail class because it's based on a calculation of where the current page sits in the menu tree - the link can be added to the parent of the page even when the page doesn't actually show in the menu at all, the only way to add it via JavaScript would be to do an AJAX request, or calculate the active trail out of band and add it to Drupal settings or something then have javascript add it from there. It would also be JavaScript that's required to render pages to anonymous users whereas currently zero JavaScript is at all out of the box.

I see. That is different from what I worked on a few years ago. There, the navigation came from the book module, and it contained the full structure of the book. So all my JS had to do was find the current page in that tree and then climb up the tree, adding the active-trail class.