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.

It's complicated by the menu allowing you to show which levels to use. So you can have a four level menu, only show the top level, but the three levels underneath all impact the active trail.

I'm not sure what the best path forward is.

After making the switch in the UI from essentially "ignore active trail" to "show active trail", if we flip the config property to match, then we end in the position of needing to update existing block configurations and how to deal with them in layout builder. We could just update global block configs, leave LB blocks unchanged, and then in code treat the "show_active_trail" property being unset to be the same as TRUE.

Alternatively, we could leave the property as "ignore_active_trail", and then in the block config form submit, flip the form value* before saving to config. I don't know if that's done anywhere else though, and it seems counterintuitive and bad DX.

*I think there's also some consideration needed on how handle the form value because the checkbox can be disabled by states, though maybe the easiest thing would be not to disable the field and only set visible/invisible.

Add the active-trail class

I tried to avoid this when we originally started work on this issue, because the concept of the 'active trail' does not exist in the UI anywhere else in Drupal core and I'm not sure we should introduce it here.

By highlighting it as the checkbox label, it promotes the concept above where I think it should be for users (ideally not at all, but minimally introduced by this MR out of necessity). #33 / #34 show exactly how difficult it is to keep track of the difference between 'active' and 'active trail'.

Yet another option would be to add the configuration option without any UI, the problem with that is discoverability of what can be a very significant performance improvement.

Yet another option would be to add the configuration option without any UI, the problem with that is discoverability of what can be a very significant performance improvement.

Random thought would be to do some sort of check in the status report, though the detection would likely be limited to globally placed menu blocks, and maaaaybe menu blocks in LB defaults. But then being able to set the config property would require some pretty technical knowledge, whether setting via drush or the config sync UI or whatever.

Just to expand a bit:

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.

I think that

Add the active-trail class

has exactly the same problem, or is possibly slightly worse - e.g. only a tiny, tiny fraction of experienced Drupal users will know what the 'active trail' class is, so everyone will need to read the description anyway. But by making the active trail concept the main thing, it becomes more important to understand what it is, vs. deciding that 'yes, I do want my main menu to look the same on every page'.

We looked at this issue again at 📌 Drupal Usability Meeting 2025-08-15 Active . (Present: benjifisher, rkoller, simohell.)

We still think that something positive ("Add a CSS class") is clearer than something negative ("Be the same on all pages"). But you make a good point about not mentioning the "active-trail class" in the UI. Can we compromise on something less specific, like one of these?

• Add a CSS class to parent links
• Highlight the ancestors of the active menu link with a CSS class
• Add a CSS class to ancestors of the current page
• Add a CSS class to links leading to [or above] the current page

The goal is that the typical user will have to read the help text (description) once, and then the label should be enough when they return to the configuration form. So the label does not have to say too much. On the other hand, the help text should explain

• This option has an effect when the block is shown on a page listed in the block.
• It applies to all "parents" or "ancestors" of the current page that are shown in the block.
• Adding the CSS class has performance implications.

@godotislate:

I think the BC problems are easier to handle than you suggested in #36.

A boolean field effectively has 3 possible values: TRUE, FALSE, or NULL. When rendering the block and inspecting the value of the new config item, treat NULL as TRUE.

Someone (@alexpott) once commented that ... ?? TRUE had a bad "code sniff" or something like that. So probably add a code comment, explaining that this is for BC.